-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[PEP 695] Fix incorrect Variance Computation with Polymorphic Methods. #19466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[PEP 695] Fix incorrect Variance Computation with Polymorphic Methods. #19466
Conversation
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Stanislav Terliakov <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Also fixes #18334 and maybe something else. |
@sterliakov I added #18334 as a unit test. |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
@sterliakov This and a few other small PRs (#19517, #19471, #19449) have been sitting since mid-July, I'm guessing you are rather swamped. Who else should I ask for review? |
I am a bit swamped indeed, and also I don't have merge powers here anyway:) This change is really non-trivial. I have already looked at this PR and, tbh, I'm still not 100% certain that binding self to Any-filled self is the correct move here. It makes some sense to me, but I'd love to see more tests with manually verified logic. I just took another look, and IMO the following snippet will be handled incorrectly: class Mixed[T, U]:
def new[S](self: "Mixed[S, U]", key: S, val: U) -> None: ...
x = Mixed[str, int]()
x.new(object(), 0) # Should error
# So this upcast is not really an upcast and should be rejected, T should be contravariant?
y: Mixed[object, int] = x
y.new(object(), 0) # Should not error
check_contra: Mixed[str, int] = Mixed[object, int]()
check_co: Mixed[object, int] = Mixed[str, int]() It's not strictly incorrect (because resolving I didn't try to compare this to Pyright or other type checkers. I'd suggest to also ping @JukkaL for review - he authored a huge part of PEP695 implementation, including this inference. |
I tested a variation of your example pyright-playground, mypy-playground from typing import cast
class Mixed[T, U]:
def new[S](self: "Mixed[S, U]", key: S, val: U) -> None: ...
def test_co(x: Mixed[str, int]) -> Mixed[object, int]:
return x # master: ❌ PR: ✅, pyright: ✅
def test_contra(x: Mixed[object, int]) -> Mixed[str, int]:
return x # master: ✅ PR: ❌, pyright: ❌
def test_now_sub(y: Mixed[object, int]) -> None:
# str value is assignable to object type.
y.new(key=str(), val=0) # master: ✅ PR: ✅, pyright: ✅
def test_new_super(x: Mixed[str, int]) -> None:
# object type is not assignable to str variable.
x.new(key=object(), val=1) # master: ❌ PR: ❌, pyright: ❌
def test_new_upcast(x: Mixed[str, int]) -> None:
# technically, one could upcast first:
z: Mixed[object, int] = x # master: ❌ PR: ✅, pyright: ✅
z = Mixed[object, int]()
z.new(key=object(), val=1) # master: ✅ PR: ✅, pyright: ✅ So both |
Self
type on an attribute #18334testPEP695InferVariancePolymorphicMethod
testPEP695SelfAttribute
My idea for fixing it was to replace
typ = find_member(member, self_type, self_type)
withtyp = find_member(member, self_type, plain_self)
inside the functioninfer_variance
, whereplain_self
is the type of self without any type variables.To be frank, I do not myself 100% understand why it works / if it is safe, but below is my best effort explanation.
Maybe a better solution is to substitute all function variables with
UninhabitedType()
?But I am not sure how to do this directly, since the type is only obtained within
find_member
.According to the docstring of
find_member_simple
:Since
plain_self
is always a supertype of the self type, however it may be parametrized, thetyp
we get this way should be compatible with thetyp
we get using the concreteself_type
. However, by binding self only toplain_self
, it replaces substituted polymorphic variables withNever
.Examples:
With this patch:
Foo.new
becomesdef [S] (self: tmp_d.Foo[Never], arg: builtins.list[Never]) -> tmp_d.Foo[Never]
in typeops.py#L470def (arg: builtins.list[Never]) -> tmp_d.Foo[Never]
in subtypes.py#L2211Bar.new
becomesdef (arg: builtins.list[T`1]) -> tmp_d.Bar[T`1]
(✅)Without this patch:
Foo.new
becomesdef [S] (self: tmp_d.Foo[T`1], arg: builtins.list[T`1]) -> tmp_d.Foo[T`1]
in typeops.py#L470 (❌)def (arg: builtins.list[T`1]) -> tmp_d.Foo[T`1]
in subtypes.py#L2211 (❌)Bar.new
becomesdef (arg: builtins.list[T`1]) -> tmp_d.Bar[T`1]
(✅)Another way to think about it is we can generally assume a signature of the form:
Now, given
self_type
isClass[T]
, it first solvesClass[T] = Class[TypeForm[S, T]]
forS
insidebind_self
, giving us some solutionS(T)
, and then substitutes it giving us some non-polymorphic methoddef method(self: Class[T], arg: TypeForm[T]) -> TypeForm[T]
and then drops the first argument, so we get the bound method
method(arg: TypeForm[T]) -> TypeForm[T]
.By providing the
plain_self
, the solution we get isS = Never
, which solve the problem.